GridCore - DataController - Add types (Part 3) - #34693
Conversation
There was a problem hiding this comment.
Pull request overview
This PR continues the GridCore typing effort by introducing/propagating stronger TypeScript types across the grid data pipeline (DataController, ColumnsController, DataSourceAdapter) and aligning handler naming for customizeStoreLoadOptions.
Changes:
- Introduces shared
LoadOperation/StoreLoadOptionstype definitions for the internal DataSource layer and extends them for grid-specific needs. - Tightens types in GridCore controllers/adapters (e.g.,
ColumnsChanges,HandleDataChangedEvent) and updates related event handler signatures. - Renames the DataSourceAdapter customization hook to
_customizeStoreLoadOptionsHandlerand updates TreeList override accordingly.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/devextreme/js/__internal/grids/tree_list/data_source_adapter/m_data_source_adapter.ts | Renames TreeList adapter hook to match the new base handler name. |
| packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/types.ts | Adds grid-specific load operation types and remote operation typing helpers. |
| packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/m_data_source_adapter.ts | Types the customize-store-load-options pipeline and renames the handler. |
| packages/devextreme/js/__internal/grids/grid_core/data_controller/types.ts | Renames the data-changed event type to HandleDataChangedEvent and updates unions. |
| packages/devextreme/js/__internal/grids/grid_core/data_controller/data_controller.ts | Applies new types to DataController handlers and improves some guard logic. |
| packages/devextreme/js/__internal/grids/grid_core/columns_controller/types.ts | Introduces ColumnsChanges shape for columns change notifications. |
| packages/devextreme/js/__internal/grids/grid_core/columns_controller/m_columns_controller.ts | Types columnsChanged and _columnChanges with ColumnsChanges. |
| packages/devextreme/js/__internal/grids/grid_core/columns_controller/m_columns_controller_utils.ts | Types updateColumnChanges inputs and normalizes option-name typing. |
| packages/devextreme/js/__internal/grids/grid_core/ai_column/controllers/m_ai_column_controller.ts | Updates AI column controller handler types for renamed data-changed event shape. |
| packages/devextreme/js/__internal/data/data_source/types.ts | Adds internal DataSource load operation/store load option type definitions. |
| packages/devextreme/js/__internal/data/data_source/m_data_source.ts | Adds the return type for _createLoadOperation and imports the new type. |
|
|
||
| that._dataChangedHandler = that._handleDataChanged.bind(that); | ||
| that._customizeStoreLoadOptionsHandler = that._handleCustomizeStoreLoadOptions.bind(that); | ||
| that._customizeStoreLoadOptionsHandlerProxy = that._customizeStoreLoadOptionsHandler.bind(that); |
There was a problem hiding this comment.
couldn't remove this proxy property, because _customizeStoreLoadOptionsHandler is extended by treelist, so it has to be a method and not an arrow func
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (3)
packages/devextreme/js/__internal/grids/tree_list/data_source_adapter/m_data_source_adapter.ts:340
- The DataSource
customizeStoreLoadOptionsevent fires with a single argument (the load operation), so calling the base handler viaapply(this, arguments as any)is unnecessary and forces ananycast. Calling the base handler directly keeps the override typed/cleaner and avoids relying onarguments.
super._customizeStoreLoadOptionsHandler.apply(this, arguments as any);
packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/types.ts:31
RemoteOperationsOptionsis introduced butLoadOperation.remoteOperationsis still typed as the widerRemoteOperationsunion (which can includeboolean/Mode). Since the adapter logic uses.filtering/.sorting/...on this value, typing it asRemoteOperationsOptionsmakes the contract clearer and avoids propagating an unusably-wide type.
remoteOperations?: RemoteOperations;
packages/devextreme/js/__internal/grids/grid_core/columns_controller/m_columns_controller_utils.ts:617
ColumnsChanges['changeTypes']includes payload-like keys (event,virtualColumnsScrolling) that are not boolean flags, butupdateColumnChangestreats everychangeTypeas a boolean toggle (changeTypes[changeType] = true). Narrow thechangeTypeparameter to only the boolean flag keys to prevent accidentally overwriting payload values in the future.
changeType: Exclude<keyof ColumnsChanges['changeTypes'], 'length'>,
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7512
- The cleanup in the finally block unsubscribes
dataSource._dataLoadingHandler, but this property is not defined anywhere (only_customizeStoreLoadOptionsHandlerProxyexists). Passingundefinedto.off(...)can remove all handlers for the event and makes the test cleanup unreliable; it should remove the proxy handler that was attached for the test and restore the original handler reference.
dataSource._dataSource.off('customizeStoreLoadOptions', dataSource._dataLoadingHandler);
dataSource._dataSource.on('customizeStoreLoadOptions', originalDataLoadingHandler);
}
There was a problem hiding this comment.
Please, check, after moving types RemoteOperations, RemoteOperationsOptions should we also move to data_source_adapter utilities isCustomStore, isLocalStore, normalizeRemoteOperations or their proper place inside data_controller
There was a problem hiding this comment.
Looks like a valid improvement to me, I have moved them in the separate commit please check it
| @@ -97,7 +95,7 @@ interface UpdateChange extends DataChangeBase { | |||
| export type DataChange = | UpdateChange | |||
There was a problem hiding this comment.
This type has grown quite complicated
A bit confusing that it is not discriminated union - changeType may be optional, also for 'refresh' it has 3 different signatures.
Please, review, whether some of options can be combined and optional marks
There was a problem hiding this comment.
Yep, it's quite complicated, but it reflects the real usage of of calling dataController.updateItems.
changeType=='refresh' appears across different signature, because it is a default value in case changeType is not set:
I think it's hard to make this type much simpler at the current stage. But I can try to make changeType be not optional
There was a problem hiding this comment.
So, I have made changeType to be always required + removed HandleDataChangedEvent type and added ChangedEvent to dataSource types instead
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Suppressed comments (3)
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7512
- The test re-subscribes
customizeStoreLoadOptionswithdataSource._customizeStoreLoadOptionsHandlerProxy, but thefinallyblock unsubscribesdataSource._dataLoadingHandler(which does not exist anywhere else in the codebase). This leaves the test-installed handler subscribed and can cause later tests to observe the modified handler.
dataSource._dataSource.off('customizeStoreLoadOptions', dataSource._dataLoadingHandler);
dataSource._dataSource.on('customizeStoreLoadOptions', originalDataLoadingHandler);
}
packages/devextreme/js/__internal/grids/grid_core/virtual_scrolling/m_virtual_scrolling.ts:240
- Leftover marker comment
// MYTODOshould be removed (it can fail text-lint rules and doesn’t provide actionable context).
// MYTODO
const callBase = super._dataChangedHandler.bind(this);
packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/types.ts:53
ChangedEvent.changeTypeis typed only as'loadError', but grid internals (e.g. legacy virtual scrolling) emit and consume other values like'append','prepend', and'pageIndex', and also rely on flags likeisDelayedand payload likeitems. The current type/comment is incomplete and will keep forcingts-expect-erroror unsafe casts.
export interface ChangedEvent extends BaseChangedEvent {
// When virtual scrolling with scrolling.legacyMode, changeType
// also can be 'append', 'prepend', 'pageIndex' in case of
changeType?: 'loadError';
error?: unknown;
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 24 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7512
- The
finallycleanup detachesdataSource._dataLoadingHandler, but this property isn’t defined on the adapter (and isn’t the handler you attached). This leaves the temporarycustomizeStoreLoadOptionshandler subscribed and then re-subscribes the original handler, resulting in two active handlers.
dataSource._dataSource.off('customizeStoreLoadOptions', dataSource._dataLoadingHandler);
dataSource._dataSource.on('customizeStoreLoadOptions', originalDataLoadingHandler);
}
packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/types.ts:52
ChangedEvent.changeTypeis documented here as also being'append' | 'prepend' | 'pageIndex'(andgrid_core/virtual_data_loader/m_virtual_data_loader.tsactually fires these values), but the type only allows'loadError'. This makes the type misleading and forces downstream code to cast/ignore type checks in legacy virtual scrolling paths.
// When virtual scrolling with scrolling.legacyMode, changeType
// also can be 'append', 'prepend', 'pageIndex' in case of
changeType?: 'loadError';
error?: unknown;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (4)
packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/utils/tests/remoteOperations.test.ts:35
- The test expects
normalizeRemoteOperations(false, …)to returnfalse, but the implementation normalizes falsy values (includingfalse) to an empty options object ({}). As written, this test will fail and also conflicts with the function'sRemoteOperationsOptionsreturn type.
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7512 - The cleanup unsubscribes using
dataSource._dataLoadingHandler, but that property is not defined anywhere in the codebase. With DevExtreme's EventsStrategy, passing an undefined handler empties all callbacks for the event, which is brittle and obscures intent. Unsubscribe the wrapper handler explicitly and restore the original proxy reference.
} finally {
dataSource._dataSource.off('customizeStoreLoadOptions', dataSource._dataLoadingHandler);
dataSource._dataSource.on('customizeStoreLoadOptions', originalDataLoadingHandler);
}
packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/types.ts:53
ChangedEvent.changeTypeis typed as only'loadError', but virtual scrolling (legacy mode) emits additional change types like'append'and'prepend'(seegrid_core/virtual_data_loader/m_virtual_data_loader.ts). The current typing contradicts actual runtime values and the comment is incomplete.
export interface ChangedEvent extends BaseChangedEvent {
// When virtual scrolling with scrolling.legacyMode, changeType
// also can be 'append', 'prepend', 'pageIndex' in case of
changeType?: 'loadError';
error?: unknown;
packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/m_data_source_adapter.ts:653
- This comment is inaccurate:
e.changeTypeis also set by_handleLoadError()(to'loadError') even whenscrolling.legacyModeis disabled. This can mislead future changes aroundisDataLoading.
// At this stage e.changeType can be defined only if virtual scrolling and scrolling.legacyMode is true
const isDataLoading = !e || isDefined(e.changeType);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (5)
packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7512
- The test replaces the
customizeStoreLoadOptionshandler withdataSource._customizeStoreLoadOptionsHandlerProxy, but thefinallyblock unsubscribesdataSource._dataLoadingHandler(which is not defined anywhere in this file). This leaves the overridden handler subscribed and can leak state into subsequent tests.
} finally {
dataSource._dataSource.off('customizeStoreLoadOptions', dataSource._dataLoadingHandler);
dataSource._dataSource.on('customizeStoreLoadOptions', originalDataLoadingHandler);
}
packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/m_data_source_adapter.ts:109
DataSourceAdapter.initdeclaresdataSource?as optional, but the implementation immediately dereferences it (dataSource.store(),dataSource.isLastPage()). This can crash ifinit()is ever called without an argument; the signature should reflect thatdataSourceis required (or add a guard).
public init(dataSource?) {
const that = this;
that._dataSource = dataSource;
that._remoteOperations = normalizeRemoteOperations(
this.option('remoteOperations'),
dataSource.store(),
);
packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/m_data_source_adapter.ts:86
DataSource._fireChanged()can fire with no argument (it calls_fireChanged()after a successful load), so thechangedhandler proxy should accept an optional event. Keeping it required makes the handler type inaccurate and encourages unsafe assumptions aboutebeing always defined.
private _dataChangedHandlerProxy!: (e: ChangedEvent) => void;
private _customizeStoreLoadOptionsHandlerProxy!: (e: LoadOperation) => void;
packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/types.ts:53
ChangedEvent.changeTypeis documented here as possibly including values likeappend/prepend/pageIndex, but the type currently only allows'loadError'. This is inconsistent with the comment and with virtual scrolling code paths that use otherchangeTypevalues.
export interface ChangedEvent extends BaseChangedEvent {
// When virtual scrolling with scrolling.legacyMode, changeType
// also can be 'append', 'prepend', 'pageIndex' in case of
changeType?: 'loadError';
error?: unknown;
}
packages/devextreme/js/__internal/grids/grid_core/data_controller/data_controller.ts:159
DataSourcecan raise thechangedevent without an argument, so the proxied handler type should accept an optional event. Having it required is misleading and can hide cases whereeisundefined.
private _dataPushedHandler!: (changes: unknown) => void;
private _dataChangedHandlerProxy!: (e: ChangedEvent) => void;
No description provided.